feat: add reusable brutalist loader system with global switch logic#72
Conversation
🚀 PR Received SuccessfullyHello @Ushnika09, Thank you for taking the initiative to contribute to this project. Please ensure that your PR follows all project guidelines properly before requesting review.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughReplaces the removed ChangesLoader Component Redesign and Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
frontend/src/components/shared/loaders/LoaderSwitcher.jsx (1)
6-6: ⚡ Quick winConsider making the loader switch configurable.
The boolean flag is hard-coded, requiring a code change and redeployment to switch loaders globally. For easier testing and configuration across environments, consider using an environment variable or accepting a prop.
🔧 Proposed enhancement with environment variable
// Centralized loader switching logic -// Change this boolean to switch loaders globally -const USE_ALT_LOADER = false; // Set to true to use the alternative loader +// Set VITE_USE_ALT_LOADER=true in .env to switch loaders globally +const USE_ALT_LOADER = import.meta.env.VITE_USE_ALT_LOADER === 'true';Then you can toggle loaders in
.envwithout code changes:# .env VITE_USE_ALT_LOADER=true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/shared/loaders/LoaderSwitcher.jsx` at line 6, Replace the hard-coded constant USE_ALT_LOADER with a configurable source: read from an environment variable (e.g., import.meta.env.VITE_USE_ALT_LOADER) and/or accept a prop on the LoaderSwitcher component so callers can override it; update where USE_ALT_LOADER is referenced in LoaderSwitcher.jsx to use the env value (coerced to boolean) with the prop taking precedence, and document the new VITE_USE_ALT_LOADER env var behavior.frontend/src/components/shared/loaders/LoaderAlt.jsx (1)
1-1: ⚡ Quick winAlign component name with filename.
The internal component is named
Spinnerbut the file isLoaderAlt.jsx. This inconsistency can confuse developers inspecting the component tree or debugging.📝 Proposed fix
-const Spinner = () => { +const LoaderAlt = () => { return (-export default Spinner; +export default LoaderAlt;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/shared/loaders/LoaderAlt.jsx` at line 1, The component name inside the file is inconsistent: the function is declared as Spinner but the file is LoaderAlt.jsx; rename the component to match the file (e.g., change the function/component declaration from Spinner to LoaderAlt) and update any default export or referenced name (component declaration and export) so the component tree and debugging show LoaderAlt consistently.frontend/src/components/shared/loaders/LoaderPrimary.jsx (1)
1-1: ⚡ Quick winAlign component name with filename.
The internal component is named
Spinnerbut the file isLoaderPrimary.jsx. This inconsistency can confuse developers inspecting the component tree or debugging.📝 Proposed fix
-const Spinner = () => { +const LoaderPrimary = () => { return (-export default Spinner; +export default LoaderPrimary;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/shared/loaders/LoaderPrimary.jsx` at line 1, The component defined as Spinner inside LoaderPrimary.jsx should be renamed to match the filename: update the component declaration and any export to use LoaderPrimary (and set LoaderPrimary.displayName = 'LoaderPrimary' if needed) so the component tree and debugging reflect the correct name; also update any local references or imports that use Spinner to use LoaderPrimary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/components/shared/loaders/LoaderAlt.jsx`:
- Around line 1-38: The Spinner component currently has no accessible text or
ARIA attributes; update the top-level container in the Spinner function to
expose loading state to assistive tech by adding role="status" and
aria-live="polite" (or aria-busy="true") and include a visually-hidden
descriptive element (e.g., a <span className="sr-only">Loading…</span>) inside
the container so screen readers announce the loader; ensure the project has a
.sr-only utility in global CSS or use inline styles for visually-hidden behavior
and keep the existing visual markup/animations (the change targets the Spinner
component's root div and its children).
In `@frontend/src/components/shared/loaders/LoaderPrimary.jsx`:
- Around line 1-104: The Spinner component currently renders only visual
elements (Spinner, class names animate-*) and lacks any accessible text or ARIA
attributes; update Spinner to include an offscreen screen-reader message (e.g.,
a span with visually-hidden/sr-only text like "Loading…" placed inside the root
div) and add role="status" and aria-live="polite" to the same container so
assistive tech announces the loading state; if .sr-only (or visually-hidden) is
not present in global CSS, add the provided sr-only utility rules to your global
stylesheet.
In `@frontend/src/pages/DashboardPage.jsx`:
- Line 14: Remove the unused debug constant by deleting the declaration "const
load = true" in the DashboardPage.jsx component (the unused symbol is "load");
if it was intended for a feature, either wire it into the component state/logic
(e.g., useState or props) and use it where appropriate, otherwise simply remove
the line to eliminate the dead variable.
---
Nitpick comments:
In `@frontend/src/components/shared/loaders/LoaderAlt.jsx`:
- Line 1: The component name inside the file is inconsistent: the function is
declared as Spinner but the file is LoaderAlt.jsx; rename the component to match
the file (e.g., change the function/component declaration from Spinner to
LoaderAlt) and update any default export or referenced name (component
declaration and export) so the component tree and debugging show LoaderAlt
consistently.
In `@frontend/src/components/shared/loaders/LoaderPrimary.jsx`:
- Line 1: The component defined as Spinner inside LoaderPrimary.jsx should be
renamed to match the filename: update the component declaration and any export
to use LoaderPrimary (and set LoaderPrimary.displayName = 'LoaderPrimary' if
needed) so the component tree and debugging reflect the correct name; also
update any local references or imports that use Spinner to use LoaderPrimary.
In `@frontend/src/components/shared/loaders/LoaderSwitcher.jsx`:
- Line 6: Replace the hard-coded constant USE_ALT_LOADER with a configurable
source: read from an environment variable (e.g.,
import.meta.env.VITE_USE_ALT_LOADER) and/or accept a prop on the LoaderSwitcher
component so callers can override it; update where USE_ALT_LOADER is referenced
in LoaderSwitcher.jsx to use the env value (coerced to boolean) with the prop
taking precedence, and document the new VITE_USE_ALT_LOADER env var behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02e5a91a-7c1b-40bd-88d6-0602012d183a
📒 Files selected for processing (7)
frontend/src/components/shared/Loader.jsxfrontend/src/components/shared/ProtectedRoute.jsxfrontend/src/components/shared/PublicRoute.jsxfrontend/src/components/shared/loaders/LoaderAlt.jsxfrontend/src/components/shared/loaders/LoaderPrimary.jsxfrontend/src/components/shared/loaders/LoaderSwitcher.jsxfrontend/src/pages/DashboardPage.jsx
💤 Files with no reviewable changes (1)
- frontend/src/components/shared/Loader.jsx
|
I agree with CodeRabbit’s suggestions regarding removing the temporary debugging logic and adding ARIA accessibility support for screen readers — both improve overall UX and accessibility The suggested changes have now been implemented. Regarding moving the loader switch boolean into environment variables/config, I don’t think it is necessary here since it is not a sensitive value and the current centralized switching logic already keeps the implementation clean and maintainable. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Excellent work on this PR. Clean architecture, strong implementation, proper reasoning, and good handling of review feedback. The centralized loader switching system was implemented professionally and maintains good scalability. Moving this to merge now. Thanks, @Ushnika09 🚀 |
kunalverma2512
added
enhancement
2 participants
Description
Implemented a reusable brutalist-style global loading system with centralized loader switching support.
Added two separate loader variants:
A centralized
LoaderSwitchercomponent was introduced to allow easy global switching between loaders using a simple boolean-based configuration without modifying existing components.The loader is integrated inside
DashboardPage.jsxand shared route loading flows for demonstration/testing purposes.Related Issue
Fixes #10
Changes Made
LoaderPrimarycomponentLoaderAltcomponentLoaderSwitcherlogicLoaderSwitcherLoom Demo
https://www.loom.com/share/ac82ebe98747444fba51248c8f6f8ab5
Summary by CodeRabbit
New Features
Style
Refactor